feat: multi-address DNS resolution for contact points and connections (DRIVER-201) - #890
feat: multi-address DNS resolution for contact points and connections (DRIVER-201)#890nikagra wants to merge 9 commits into
Conversation
…VER-201) newControlReconnectionQueryPlan() now creates copies of the original contact-point nodes (with their unresolved hostname endpoints) instead of synthetic nodes with resolved IPs. This ensures the control channel carries the hostname endpoint, which is preserved in metadata after topology refresh. DNS expansion for connection fallback is handled by ChannelFactory (PR scylladb#890), so the control-reconnection path does not need to inject resolved-IP nodes into the query plan. Also adds getContactPoints() stub back to LoadBalancingPolicyWrapperTest so tests that cover the control-reconnect path continue to pass.
Before-init query plan now uses getContactPoints() (original unresolved hostname nodes) instead of getResolvedContactPoints(). The DNS expansion to all IPs happens at the ChannelFactory level (PR scylladb#890), so expanding here was redundant and broke should_connect_with_mocked_hostname by replacing hostname endpoints with resolved-IP endpoints. Also remove the should_connect_when_first_dns_entry_is_non_responsive integration test from this PR; it belongs in PR scylladb#890 where ChannelFactory expansion actually enables it to pass.
There was a problem hiding this comment.
Pull request overview
Part 2/2 of DRIVER-201: extends the EndPoint API and ChannelFactory so that a hostname mapping to multiple IPs is tried address-by-address at the connection layer, instead of only the first IP. The EndPoint.resolve() method is deprecated in favor of a new resolveAll() default method; DefaultEndPoint, SniEndPoint, and ClientRoutesEndPoint override it; ChannelFactory.connect() now iterates over candidates and only fails when all are exhausted, while keeping protocol-version downgrade scoped to a single address.
Changes:
- Add
EndPoint.resolveAll()(default impl delegating to deprecatedresolve()); override inDefaultEndPoint,SniEndPoint,ClientRoutesEndPoint. - Rework
ChannelFactory.connect()intotryNextCandidate/connectToAddressso per-address failures fall back to the next IP while protocol-version downgrades stay scoped to one address. - Add unit tests for
DefaultEndPoint.resolveAll()and a newSniEndPointTest.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java | Deprecates resolve(); adds default resolveAll() method. |
| core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java | Overrides resolveAll() using InetAddress.getAllByName with single-address fallback. |
| core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java | Overrides resolveAll() returning one address per sorted A-record. |
| core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java | Overrides resolveAll() to wrap the single topology-monitor address. |
| core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java | Adds candidate-iteration and per-address protocol-negotiation methods. |
| core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java | New tests for resolveAll() (resolved, unresolved expansion, unresolvable fallback). |
| core/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java | New test class covering SNI resolveAll() happy path, unresolvable host, and resolve() sanity check. |
Comments suppressed due to low confidence (1)
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java:303
- When
connectToAddressfails withUnsupportedProtocolVersionException.forNegotiation(i.e. all protocol downgrades exhausted),tryNextCandidatewill treat this like any other per-address failure and try the next IP, even though the protocol-negotiation failure is a server-wide condition that will recur on every other IP of the same node. This also reuses the sharedattemptedVersionsCopyOnWriteArrayListacross candidates, so on each subsequent address the downgrade loop re-attempts the same protocol versions and adds duplicate entries, and the final exception ultimately reported will list each version multiple times. Consider distinguishing non-address-specific failures (UnsupportedProtocolVersionException, authentication errors, etc.) and short-circuiting the candidate loop in those cases.
perAddressFuture.whenComplete(
(channel, error) -> {
if (error == null) {
resultFuture.complete(channel);
} else if (index + 1 < candidates.length) {
LOG.debug(
"[{}] Failed to connect to {} ({}), trying next address",
logPrefix,
candidate,
error.getMessage());
tryNextCandidate(
endPoint,
shardingInfo,
shardId,
options,
nodeMetricUpdater,
currentVersion,
isNegotiating,
attemptedVersions,
resultFuture,
candidates,
index + 1);
} else {
// Note: might be completed already if the failure happened in initializer()
resultFuture.completeExceptionally(error);
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
05553f3 to
f631971
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Sequence Diagram(s)sequenceDiagram
participant ChannelFactory
participant EndPoint
participant tryNextCandidate
participant connectToAddress
participant resultFuture
ChannelFactory->>EndPoint: resolveAll()
EndPoint-->>ChannelFactory: SocketAddress[] candidates
ChannelFactory->>tryNextCandidate: attempt candidate at index 0
tryNextCandidate->>connectToAddress: connect using perAddressFuture
alt connection succeeds
connectToAddress-->>tryNextCandidate: DriverChannel
tryNextCandidate->>resultFuture: complete successfully
else connection or negotiation fails
connectToAddress-->>tryNextCandidate: complete perAddressFuture exceptionally
tryNextCandidate->>tryNextCandidate: attempt next candidate
end
tryNextCandidate->>resultFuture: fail after all candidates
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
f631971 to
860a34d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java`:
- Around line 222-242: The code calls endPoint.resolveAll() and passes the
resulting candidates array into tryNextCandidate() which immediately indexes
candidates[0]; guard against null or empty results by validating the output of
endPoint.resolveAll()—if it returns null or candidates.length == 0, complete
resultFuture exceptionally (or create a specific error) and return; otherwise
call tryNextCandidate(...) with the non-empty candidates. Update the block
around resolveAll(), candidates, and the call to tryNextCandidate() to perform
this check and fail fast via resultFuture.completeExceptionally when
appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ad3d5b5-6473-4c88-8777-93861f5de639
📒 Files selected for processing (12)
core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.javacore/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.javacore/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java
860a34d to
a6d0e48
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.java (1)
37-37: ⚡ Quick winConsider adding test coverage for resolveAll() throwing an exception.
The
ChannelFactory.connect()implementation includes a catch block for exceptions thrown byresolveAll()(see context snippet 1, line 232). Adding a third test case where the mockedEndPoint.resolveAll()throws an exception (e.g.,UnknownHostException) would ensure all three defensive paths are tested:
- ✓ Returns null (covered)
- ✓ Returns empty array (covered)
- ✗ Throws exception (not covered)
📋 Suggested test case
`@Test` public void should_fail_future_when_resolve_all_throws_exception() { // Given when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); ChannelFactory factory = newChannelFactory(); EndPoint badEndPoint = mock(EndPoint.class); RuntimeException testException = new RuntimeException("DNS lookup failed"); when(badEndPoint.resolveAll()).thenThrow(testException); // When CompletionStage<DriverChannel> channelFuture = factory.connect( badEndPoint, null, null, DriverChannelOptions.DEFAULT, NoopNodeMetricUpdater.INSTANCE); // Then – future must complete exceptionally with the thrown exception assertThatStage(channelFuture) .isFailed(e -> assertThat(e).isSameAs(testException)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.java` at line 37, Add a third test in ChannelFactoryResolveAllGuardTest that verifies ChannelFactory.connect() propagates exceptions thrown by EndPoint.resolveAll(): mock an EndPoint (e.g., badEndPoint) to throw a RuntimeException (or UnknownHostException) from resolveAll(), create the factory via newChannelFactory(), call factory.connect(badEndPoint, ...) with DriverChannelOptions.DEFAULT and NoopNodeMetricUpdater.INSTANCE, and assert the returned CompletionStage<DriverChannel> completes exceptionally with the same exception; this mirrors the existing tests for null/empty resolveAll() and targets the catch path in ChannelFactory.connect().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.java`:
- Line 37: Add a third test in ChannelFactoryResolveAllGuardTest that verifies
ChannelFactory.connect() propagates exceptions thrown by EndPoint.resolveAll():
mock an EndPoint (e.g., badEndPoint) to throw a RuntimeException (or
UnknownHostException) from resolveAll(), create the factory via
newChannelFactory(), call factory.connect(badEndPoint, ...) with
DriverChannelOptions.DEFAULT and NoopNodeMetricUpdater.INSTANCE, and assert the
returned CompletionStage<DriverChannel> completes exceptionally with the same
exception; this mirrors the existing tests for null/empty resolveAll() and
targets the catch path in ChannelFactory.connect().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b702fd48-9ba7-4994-8bb9-351438fb02a8
📒 Files selected for processing (13)
core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.javacore/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.javacore/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java
✅ Files skipped from review due to trivial changes (5)
- core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java
- core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.java
- core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.java
- core/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.java
🚧 Files skipped from review as they are similar to previous changes (7)
- core/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java
- core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java
- core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java
a6d0e48 to
f9265b3
Compare
|
🤖: Valid nitpick. Added a third test |
f9265b3 to
4448119
Compare
4448119 to
1c8dfa2
Compare
|
Rebased this PR (Part 2/2) on top of #889 ( Also addressed the outstanding review feedback:
Previously-addressed items (Copilot / CodeRabbit) remain in place after the rebase: N×timeout Javadoc on Verified locally on JDK 11: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java:62
NETTY_ADMIN_SIZEonly configures the number of admin event-loop threads (DefaultDriverOption.java:807-811); it does not configure anAddressResolverGroup. This link gives users an incorrect way to identify or change the resolver. Refer to a customNettyOptionsbootstrap hook instead, or omit the configuration link.
* <p><b>Note on resolver:</b> DNS lookup is performed via {@link
* InetAddress#getAllByName(String)} on the calling thread, bypassing any custom Netty {@code
* AddressResolverGroup} configured via {@link
* com.datastax.oss.driver.api.core.config.DefaultDriverOption#NETTY_ADMIN_SIZE}. This is
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java`:
- Line 603: Update the public Javadoc for the reconnection-plan option in
TypedDriverOption to state that it appends DNS-expanded candidates returned by
getResolvedContactPoints(), rather than raw original contact points, and that
monitors which re-resolve addresses skip this behavior; retain the documented
default of true.
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java`:
- Around line 147-153: Prevent blocking DNS resolution from query-plan creation
by moving MetadataManager.getResolvedContactPoints() off the caller thread or
introducing bounded caching before using its results. Apply the fix to the
BEFORE_INIT/DURING_INIT path in
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java:147-153
and the control-reconnect path in
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java:164-184;
update core/src/main/resources/reference.conf:2321-2334 if needed so
fallback-to-original-contact-points is not enabled without bounded, non-blocking
resolution.
In `@core/src/main/resources/reference.conf`:
- Around line 2321-2334: The default for fallback-to-original-contact-points
must not enable the blocking DNS fallback path; change this configuration
default back to false while preserving the existing setting name and
documentation.
In
`@core/src/test/java/com/datastax/oss/driver/internal/core/metadata/MetadataManagerTest.java`:
- Around line 512-529: The test should enforce expansion to the complete DNS
result set, not merely verify that one resolved node exists. Update
should_expand_unresolved_hostname_to_all_ips to obtain
InetAddress.getAllByName("localhost"), compare the returned node count and
endpoint addresses against all expected addresses on port 9042, and retain the
resolved-address assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 648940a1-36ee-47f0-8f02-aff008723307
📒 Files selected for processing (29)
core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.javacore/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.javacore/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.javacore/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.javacore/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/helper/OptionalLocalDcHelper.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/CloudTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/MetadataManager.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/TopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.javacore/src/main/resources/reference.confcore/src/test/java/com/datastax/dse/driver/internal/core/insights/InsightsClientTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapperTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/MetadataManagerTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/heartbeat/HeartbeatIT.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java
🚧 Files skipped from review as they are similar to previous changes (11)
- core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.java
- core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java
- core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java
- core/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java
- core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java
… (DRIVER-201) When RESOLVE_CONTACT_POINTS=false (the default) a hostname contact point was stored as a single unresolved InetSocketAddress, so the query plan tried only the first DNS IP. Keep contact points unresolved and expand each hostname to all its DNS IPs at query-plan time via MetadataManager.getResolvedContactPoints(), so the driver falls back to the next candidate when one IP is unreachable. Resolution is bounded, concurrent and best-effort. getResolvedContactPoints() runs on the admin event loop, where nothing should block, so each blocking InetAddress.getAllByName() call is offloaded to a cached daemon-thread pool and all unresolved hostnames are resolved concurrently against a single CONTACT_POINT_RESOLUTION_TIMEOUT deadline. A cached pool (rather than one shared thread) means each hostname resolves on its own thread, so one slow or blackholed lookup cannot starve the sibling contact points, nor the next reconnect that would otherwise queue behind it. If a hostname cannot be resolved or resolution times out, the original unresolved contact point is kept as-is rather than dropped, so the query plan is never emptier than the configured contact points and the address can still be resolved later at connection time (as it was before DNS expansion existed). This is an interim mitigation, superseded by scylladb#890's non-blocking EndPoint.resolveAll(). Default advanced.control-connection.reconnection.fallback-to-original-contact-points to true (no longer Experimental): it is the DNS re-resolution path on reconnect. Metadata nodes hold an already-resolved endpoint that is never re-resolved, so falling back to the original unresolved contact points re-expands the hostname to its current DNS IPs. Document that DNS-expanded contact points are IP-backed connection candidates that may be persisted in metadata, and that each synthetic endpoint retains the original hostname (built from the resolved InetAddress) so TLS peer host / SNI / hostname verification keep using the configured hostname. Gate the control-connection reconnection contact-point fallback behind a new TopologyMonitor.reresolvesNodeAddresses() (default false; true for the proxy-based ClientRoutesTopologyMonitor and CloudTopologyMonitor). Those monitors reach nodes through endpoints that already re-resolve on every connection attempt and maintain an authoritative node set, so appending raw contact points to their reconnection plan is unnecessary and could resurrect removed nodes (PrivateLink/Cloud regression safety). The reconnection plan also appends the contact points only once the load balancing policy is RUNNING, so the pre-init plan (already built from the resolved contact points) is not duplicated or re-resolved. Remove OptionalLocalDcHelper.checkLocalDatacenterCompatibility(): it warned when a contact point reported a different datacenter than the configured local DC. Since commit 12e6acb switched initial metadata refresh to hostId-only matching, contact-point nodes are never reused and their datacenter stays null; comparing a configured local DC against that null made the check fire as a false positive for every contact point whenever local-datacenter was set on the default profile, rather than surface a real mismatch. The node-based "configured local DC matches no node" warning (against discovered nodes whose datacenters are populated) is retained, so the only user-visible effect is that the spurious warning is no longer emitted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (4)
core/src/main/java/com/datastax/oss/driver/internal/core/util/AddressUtils.java:83
- A resolved IPv6 literal can be misclassified as a hostname here. For example,
new InetSocketAddress("::1", port)retains::1as its host string, whilegetHostAddress()normalizes it to0:0:0:0:0:0:0:1, so this returnstrue. That makesSniEndPointconvert an IP proxy back to an unresolved name and can makereattachHostnamelabel redirected candidates with an IP literal belonging to a different address. Parse the original host spelling for both resolved and unresolved addresses instead of comparing normalized strings.
InetAddress ip = address.getAddress();
return ip != null
? !hostString.equals(ip.getHostAddress())
: !InetAddresses.isInetAddress(hostString);
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java:710
- This says expansion happens at query-plan time, but this PR deliberately keeps one unresolved contact-point node in the plan and expands it in
ChannelFactoryat connection time. Correcting the public option documentation avoids describing the superseded #889 design.
* <p>This is also the driver's DNS re-resolution path: contact points are expanded to their
* current DNS IPs at query-plan time, whereas metadata nodes hold an already-resolved endpoint
* that is never re-resolved. Keeping this enabled lets control-connection reconnects re-resolve
* the original hostnames and pick up new IPs once the live-node plan is exhausted.
upgrade_guide/README.md:53
- The unconditional “no further expansion” statement conflicts with the new resolver contract: custom resolvers are consulted for already-resolved addresses and may redirect or expand them. Please qualify this as the default resolver behavior, consistent with
ChannelFactoryNettyResolverTest.should_let_the_resolver_redirect_an_already_resolved_address.
core/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.java:177 - “No further expansion” is not true for a custom Netty resolver:
ChannelFactory.resolveCandidates()consults the resolver even for an address that already carries an IP, and the new redirect test explicitly relies on it being able to report that address unresolved and return multiple/substitute candidates. Qualify this as the normal/default-resolver behavior so callers do not assume a resolved address bypasses their configured resolver.
* int)} to opt in) and to hostnames specified in the configuration. An already-resolved address
* passed here (the common case when constructing an {@code InetSocketAddress} directly from a
* hostname, which resolves eagerly) is used as provided, with no further expansion. The {@code
* advanced.resolve-contact-points} option is deprecated and has no effect.
|
Two more commits, from reviewing the finished PR rather than from a thread — no production defect, but one gap worth closing before this merges.
((InetSocketAddress) node.getEndPoint().resolve()).getAddress().getHostAddress()throws
Same commit: an upgrade-guide bullet for the two
Not fixed here, filed as #989: duplicate resolver candidates are not collapsed, so a name with a repeated A record spends an extra full Verified on JDK 11: 3853 core unit tests, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java:710
- This public option documentation says expansion happens at query-plan time, but this PR deliberately keeps the query plan unresolved and expands addresses in
ChannelFactoryat connection time. Correct the timing so the API docs match the implementation and the rest of the migration guide.
* <p>This is also the driver's DNS re-resolution path: contact points are expanded to their
* current DNS IPs at query-plan time, whereas metadata nodes hold an already-resolved endpoint
* that is never re-resolved. Keeping this enabled lets control-connection reconnects re-resolve
* the original hostnames and pick up new IPs once the live-node plan is exhausted.
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java:218
- This also marks numeric route values such as
127.0.0.1as unresolved. IfBootstrap.disableResolver()is configured,resolveCandidates()passes that unresolved socket through and Netty fails withUnresolvedAddressException, even though an IP literal requires no lookup; the previousInetAddress.getByName()path produced a resolved address. Preserve literals as resolved and defer only actual hostnames.
return InetSocketAddress.createUnresolved(route.getHostname(), route.getPort());
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/TopologyMonitor.java:154
- The contract says contact points must never be appended when this returns
true, butnewControlReconnectionQueryPlan()deliberately appends them when the regular plan is empty. Document that exception so custom monitor implementations can reason about the actual behavior.
* <p>When this returns {@code true}, the control connection's reconnection query plan must not
* append the original contact points as a DNS re-resolution fallback (see {@code
* advanced.control-connection.reconnection.fallback-to-original-contact-points}): the monitor
* already keeps addresses fresh, and appending raw contact points could resurrect nodes that the
* monitor has authoritatively removed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (3)
core/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.java:177
- The “no further expansion” guarantee is not true with a custom Netty resolver:
ChannelFactory.resolveCandidates()always asks the resolver, and the addedshould_let_the_resolver_redirect_an_already_resolved_addresstest explicitly verifies that it can classify and expand/redirect an already-resolved address. Qualify this as the normal/default-resolver behavior so callers do not rely on a guarantee the connection layer does not provide.
* int)} to opt in) and to hostnames specified in the configuration. An already-resolved address
* passed here (the common case when constructing an {@code InetSocketAddress} directly from a
* hostname, which resolves eagerly) is used as provided, with no further expansion. The {@code
* advanced.resolve-contact-points} option is deprecated and has no effect.
upgrade_guide/README.md:68
- This upgrade note also overstates that an already-resolved programmatic address receives no further expansion. The connection layer consults the configured resolver for resolved addresses too, and a custom resolver can classify one as unresolved and redirect or expand it. Please document that exception consistently with the actual Netty-compatible behavior.
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java:710 - This public option documentation says contact points are expanded at query-plan time, but the new implementation deliberately keeps one unresolved node in the plan and expands it in
ChannelFactory.resolveCandidates()at connection time. Please correct the timing so API consumers are not given the superseded design.
* <p>This is also the driver's DNS re-resolution path: contact points are expanded to their
* current DNS IPs at query-plan time, whereas metadata nodes hold an already-resolved endpoint
* that is never re-resolved. Keeping this enabled lets control-connection reconnects re-resolve
* the original hostnames and pick up new IPs once the live-node plan is exhausted.
| ConsistencyLevel.LOCAL_QUORUM.name())); | ||
| } | ||
| } | ||
| perAddressFuture.complete(driverChannel); |
There was a problem hiding this comment.
Completing the per-address future here discards the remaining DNS candidates before ControlConnection resolves the contact point through system.local. If that lookup fails, the query plan advances past the hostname's single node, so a healthy secondary address is never tried. Keep the candidates retryable until control-node resolution succeeds.
There was a problem hiding this comment.
Agreed, fixed in ff9d8a5095 — but in ControlConnection, not by keeping the candidates alive across the layer boundary. On an identity-read failure it now retries the same query plan entry, and rotate()'s per-connect counter puts the next attempt on another address. Gated on hostId == null + AddressUtils.carriesName, and it stops as soon as a pinned address comes back round.
| // address it ever connected to, even after the control connection moved to another one and told | ||
| // us about it. | ||
| endPoint = newEndPoint; | ||
| if (differentMetricIdentity) { |
There was a problem hiding this comment.
This newly added equal-endpoint rebuild path removes its replacement metrics. After endPoint is changed and the new updater registers, Dropwizard/MicroProfile's previousMetricUpdater.clearMetrics() recomputes IDs from the node's new endpoint, deleting the new series and leaving the old ones. Clear the previous updater while the node still has the old endpoint, then swap and register.
There was a problem hiding this comment.
Confirmed and fixed in 3326a5fdad — now clear, swap, build. One nit on the framing: the ordering is upstream's verbatim; this branch only widened the trigger from !equals to metric identity, which is what brings the ordinary contact-point transition onto it. Micrometer is unaffected (removes Meter instances). New DefaultNodeTest case drives a real MetricRegistry.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (5)
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java:83
InetSocketAddress.equals()does not distinguish IPv6 scope IDs: two link-local addresses with identical 128-bit bytes and ports but different zones compare equal. A custom resolver can redirect an already-resolved scoped address to the same bytes on another interface; this shortcut then returns the original endpoint even though the channel connects using the candidate's different scope, violating the pin-to-connected-address invariant. Use a scope-aware address comparison (includingInet6Address.getScopeId()) for both no-op checks.
|| resolvedAddress.equals(this.pinnedAddress)
// The address we already hold: pinning to it changes nothing, since resolve() and
// toString() would keep yielding what they already do. Skipping the copy spares toString()
// a
// redundant "addr(addr)" suffix on every already-resolved endpoint -- which is all of them,
// once a node is discovered from the peers rows.
|| resolvedAddress.equals(this.address)) {
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java:124
- These equality shortcuts collapse scoped IPv6 addresses from different interfaces because
InetSocketAddress.equals()ignores theInet6Addressscope ID. If a custom resolver redirects an IP proxy to the same link-local bytes in another zone, the endpoint remains pinned to the old zone while TCP connects to the new one. Compare scope IDs in addition to normal socket-address equality.
|| resolvedAddress.equals(this.pinnedAddress)
// The address we already hold: pinning to it changes nothing -- resolve() and toString()
// keep yielding what they already do -- so spare the copy (and its redundant
// "proxy(proxy)" toString suffix). Only reachable when the proxy was given as an IP
// address: a proxy hostname is stored unresolved, and a resolved pin never compares equal
// to that.
|| resolvedAddress.equals(this.proxyAddress)) {
core/src/main/java/com/datastax/oss/driver/internal/core/control/ControlConnection.java:619
triedAddressesis aHashSet<SocketAddress>, butInet6Address.equals()/hashCode()ignore the scope ID. If a hostname yields the same link-local bytes on two interfaces and identity resolution fails on the first, adding the second scoped address returns false and advances the query plan without trying that distinct destination. Track a scope-aware key (address bytes, port, and IPv6 scope) instead.
&& triedAddresses.size() < MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY
&& expandsToSeveralAddresses(node.getEndPoint())
&& triedAddresses.add(triedAddress)) {
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java:710
- This still says contact points are expanded at query-plan time, but the new implementation deliberately keeps one unresolved node in the plan and expands it in
ChannelFactoryat connection time. Correcting this avoids contradicting the option's actual behavior and the rest of the PR documentation.
* <p>This is also the driver's DNS re-resolution path: contact points are expanded to their
* current DNS IPs at query-plan time, whereas metadata nodes hold an already-resolved endpoint
* that is never re-resolved. Keeping this enabled lets control-connection reconnects re-resolve
* the original hostnames and pick up new IPs once the live-node plan is exhausted.
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java:116
- The pinned-address no-op check is not exact for scoped IPv6:
InetSocketAddress.equals()ignores the zone/scope ID. Re-pinning an identified client-route node from one interface to the same link-local bytes on another therefore returns the stale pinned endpoint, so SSL and topology code observe a different address from the channel's actual destination. Include the IPv6 scope ID in this comparison.
if (!(resolvedAddress instanceof InetSocketAddress)
|| resolvedAddress.equals(this.pinnedAddress)) {
…VER-201) OptionalLocalDcHelper.checkLocalDatacenterCompatibility() warned when a contact point reported a datacenter different from the configured local DC. This has been dead code on scylla-4.x since 12e6acb: refresh matches nodes by hostId only, so contact-point nodes never get a datacenter assigned and the warning could never fire. Remove it. The separate "configured local DC matches no node" warning is retained. Nothing covered the removal, and CUSTOMER-588 is the bug the check caused: it compared the configured local DC against ephemeral placeholder Nodes (built by MetadataManager#addContactPoints via DefaultNode#newContactPoint, datacenter always null), so it warned unconditionally whenever a local DC was configured, no matter where the contact points actually were. The new test builds a placeholder Node the same way production does, plus a resolved node that genuinely is in the configured local DC, and asserts no warning is logged. It asserts on the absence of any WARN rather than of one particular message, so a regression under different wording is still caught; should_warn_if_configured_dc_matches_no_node is the positive control for the same appender, so a silent capture failure cannot make it pass by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…S (DRIVER-201) Contact points backed by a hostname are now always kept unresolved, so the connection layer can expand them to all their DNS-mapped IPs at connection time. SessionBuilder no longer reads RESOLVE_CONTACT_POINTS when merging contact points; the option is deprecated and has no effect. An already-resolved InetSocketAddress passed programmatically is still used as provided, with no further expansion. OptionsMap.fillWithDriverDefaults() still carries the option's reference.conf value so the defaults map stays complete, and is annotated accordingly -- the build treats deprecation warnings as errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Groundwork for expanding a hostname to all of its addresses: resolution becomes
the connection layer's job, so everything that produces an EndPoint stops doing
DNS of its own, and an endpoint gains a way to record which address a connection
actually reached.
PinnableEndPoint is the new internal contract: pinTo(SocketAddress) returns a
copy bound to one address, and the pin is excluded from equals(), hashCode(),
asMetricPrefix() and toString(). Endpoints are set and map keys, and node
metrics are named after them, so a pinned copy has to be indistinguishable from
its original everywhere except when the connection layer asks which address
answered. A generic delegating wrapper was rejected: its equals() would be
asymmetric, because DefaultEndPoint#equals tests instanceof and would reject the
wrapper while the wrapper accepted the original, and it would break
SniSslEngineFactory's instanceof SniEndPoint guard. Each implementation
therefore carries a nullable pinnedAddress of its own.
SniEndPoint additionally normalizes a resolved proxy *hostname* back to
unresolved. withCloudProxyAddress(new InetSocketAddress("proxy", 9042)) resolves
eagerly, which froze Cloud on whichever proxy IP the JVM happened to return; an
IP-literal proxy is left alone. Contact points keep the opposite policy on
purpose, since ContactPoints.merge() only ever applied its resolve flag to
config-file entries.
ClientRoutesTopologyMonitor.resolve() likewise returns the client route as an
unresolved address and no longer looks it up, which keeps it a pure in-memory
cache lookup that is safe to call from an event loop, and lets a custom resolver
apply to client routes just as it does to contact points. Its protected
resolveAddress() extension point, which existed only so tests could stub out
InetAddress.getByName, goes with it. This has to move together with
ClientRoutesEndPoint: dropping "throws UnknownHostException" from one and the
matching catch from the other is a single compilable change.
TopologyMonitor gains reresolvesNodeAddresses(), which tells the control
connection's reconnection query plan whether this monitor already keeps
addresses fresh. It defaults to false, correct for DefaultTopologyMonitor, whose
peers hold an already-resolved IP from system.peers. ClientRoutesTopologyMonitor
reports true only when every currently-known node actually has a live route:
where the route set is incomplete, ClientRoutesEndPoint falls back to a static
resolved endpoint, and those nodes still need the contact-point fallback.
The "is this a name" test several of these need is shared as
AddressUtils.carriesName(): a resolved address compares its host string against
the literal its own bytes produce, an unresolved one parses its host string.
Neither isUnresolved() nor the presence of an InetAddress can tell a name from a
literal on its own.
DseGssApiAuthProviderBase.serverName() falls back to getHostString() when
getAddress() returns null, which is now the ordinary case for a Cloud or
client-route endpoint rather than an impossible one.
EndPoint.resolve() keeps its signature and is not deprecated, so third-party
implementations still compile. Its javadoc gains two expectations: return the
address as-is rather than looking names up, since this is now called from an
event loop; and callers are warned that the returned address is no longer always
resolved, so getHostString() is the safe way to read the host.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…R-201) This is the fix for DRIVER-201. When a contact point or a node address is a hostname that maps to several IPs, the driver used to try only the first one and raise AllNodesFailedException if it was unreachable, even though the hostname also resolved to healthy addresses. Resolution is a connection-layer concern. ChannelFactory.connect() is now the single place that turns "the address this node is known by" into "the addresses to actually try": EndPoint.resolve() yields one address and does no lookup, so it stays safe to call from an event loop; ChannelFactory expands it through the bootstrap's Netty AddressResolverGroup; the candidates are tried in sequence until one connects; and the endpoint is pinned to the address that won, so the channel carries the address it is really on. Expansion always goes through the configured resolver, mirroring Netty's own doResolveAndConnect0 short-circuit (no group, !isSupported, isResolved) rather than pre-filtering on isUnresolved(). Both isSupported() and isResolved() are overridable, so a redirecting custom resolver keeps its say over addresses that merely look resolved. The bootstrap is built once per connect() and cloned per attempt, with the clone's resolver disabled: Bootstrap.clone() carries the resolver over, so an enabled clone would resolve each candidate a second time -- through resolve(), singular -- and a redirecting resolver would collapse every candidate onto its first answer, silently killing the fallback. Details that took a round each to get right: - The queried hostname is re-attached to resolver-returned addresses, centrally rather than per endpoint, so TLS sees the name the user configured instead of an IP or a CNAME label. Scoped IPv6 keeps its zone via the numeric Inet6Address.getByAddress overload; the NetworkInterface one re-derives the scope and throws when the interface has no address of the same local type. - One EventLoop is chosen per connect() and shared by resolution and every clone(eventLoop), instead of letting Bootstrap.connect() advance the chooser a second time and land channels on half the loops. - Candidate rotation uses a per-name counter held by the factory (per session) behind a bounded LoadingCache, since client routes can churn hostnames within one session. - Protocol-version rejection is terminal only for a node whose host id is known. The addresses of an unidentified endpoint may belong to different nodes, and collapsing a contact-point hostname into one Node must not lose the query-plan advance that resolve-contact-points=true used to provide. - Failures from earlier candidates are attached to the final error as suppressed exceptions, and negotiation history is scoped per candidate address. - Every resolver and Netty callback completes the connect future on failure. connect() has no timeout at the resolution stage, so an unguarded throw would hang the caller for good. afterBootstrapInitialized() now runs once per logical connection rather than once per attempt, and sees the bootstrap before the driver's handler is installed; a handler set by the hook is overwritten, with a one-time warning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…IVER-201) Node metrics are named after the endpoint, so DefaultNode.setEndPoint() has to re-register them whenever those names change -- which is not the same question as whether this is a different node, and the old !equals() test got it wrong in both directions. It was too narrow: an unresolved hostname and the resolved address it maps to compare *equal* while their metric prefixes differ, which is exactly what happens when a contact-point node adopts the endpoint built from its system.local row. And too wide in the other direction is now possible too, since a pinned copy differs from its original only by an address that both equals() and the metric identity ignore by contract. The test is therefore asMetricPrefix() plus toString(), because both are in use: the default MetricIdGenerator names node metrics after the prefix, the tagging one tags them with toString(). The pin is excluded from toString() as well, or DefaultTopologyMonitor#buildNodeEndPoint returning the control channel's pinned endpoint for the system.local row would silently retag node metrics on every refresh and orphan the old series. The node also adopts the newest endpoint instance even when it compares equal, since a pinned copy carries the address every subsequent connection will use. Finally, the rebuild order is clear, then swap, then build. Dropwizard and MicroProfile do not remember the ids they registered under; their clearMetrics() recomputes each one from the node's endpoint as it stands at that moment. The previous order -- swap, build, clear -- therefore deleted exactly the series the new updater had just registered and left the old ones behind with nothing writing to them. That ordering is upstream's, but it used to be reached only when the endpoints compared unequal; keying the rebuild on metric identity brings the ordinary contact-point transition onto the same path. The pre-existing pin test was vacuous: a mocked context yields NoopNodeMetricUpdater, for which the rebuild is skipped entirely. Both tests now stub MetricsFactory, and the ordering test drives a real MetricRegistry through a hostname-to-IP rename; it was proven to fail under the old order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fails (DRIVER-201) ChannelFactory walks all of a hostname's addresses while it opens a channel, but the control node's identity is only read afterwards, over the channel that won: by then the remaining candidates are gone. ControlConnection advanced its query plan on that failure -- and since a contact-point hostname is now a single Node, that wrote off the whole hostname on the strength of one of its addresses. With a single contact point and the default reconnect-on-init=false, session initialization failed outright, and a rebuilt session got a fresh ChannelFactory whose rotation counters start at zero, so it failed the same way every time while a healthy address sat unused. The addresses of an unidentified endpoint may well belong to different nodes, which is the same reason ChannelFactory#isNodeWideFailure only treats a protocol-version rejection as terminal for a node whose host id is known. So the query plan entry is attempted again instead, and the next attempt lands on another address because ChannelFactory rotates its candidates once per connect. The walk terminates on the address set alone: it only grows, a retry requires it to grow, and coming back round to an address already in it ends the walk. MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY is a backstop against a resolver that never repeats itself, and bounds how long one hostname can hold up initialization. It is tested before the address is recorded, so an entry is attempted at most one more time than the cap itself. It arms only for a node with no host id whose endpoint denotes a name and whose channel reports a resolved pinned address. Identified nodes stay pinned to one address on purpose, literals expand to exactly themselves, and a third-party EndPoint that ChannelFactory passed through without pinning cannot say which address answered -- all three keep behaving exactly as before. The landed address is captured as soon as the channel opens, because resolveChannelNodeIfNeeded() overwrites the channel's endpoint with the one built from the system.local row before the registration that can still fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n (DRIVER-201) advanced.control-connection.reconnection.fallback-to-original-contact-points now defaults to true, and is the driver's DNS re-resolution path. Nothing else re-resolves. Metadata nodes hold an endpoint built from an already-resolved system.peers IP, and the control node's own endpoint is pinned by ChannelFactory to the single address its connection reached, deliberately, so that a node with a known identity cannot wander to a different host. Once the records behind a hostname change, appending the original contact points is therefore the only way back: they are still unresolved hostnames, so ChannelFactory expands each one to its current IPs at connection time. The append is gated on the topology monitor not re-resolving addresses itself, since a proxy-based monitor keeps them fresh and raw contact points could resurrect nodes it has authoritatively removed. The exception is an empty regular plan: with no live node to try, reconnection cannot recover on its own. The plans are concatenated rather than mutated. A RUNNING-state query plan is a built-in QueryPlan whose add()/addAll() throw UnsupportedOperationException, poll() being its only mutator, so with the fallback defaulting on every post-init control reconnect would otherwise have crashed. The append is also skipped before the LBP reaches RUNNING, where newQueryPlan() has already built the plan from the contact points and appending would duplicate every entry. Documented cost: the contact points are appended without being compared against the live-node plan, because at plan time they are hostnames while the live nodes are resolved IPs. When DNS has not changed they expand to addresses the plan just failed on, so an exhausted reconnection round retries roughly twice as many addresses -- which is why HeartbeatIT has to disable it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rewrites the address-resolution manual page around the connection layer doing the expansion, and adds an upgrade-guide section covering what changes for users: - there is no public API change, but EndPoint.resolve() may now return an unresolved address for Cloud/SNI and client-route nodes, so a caller doing ((InetSocketAddress) resolve()).getAddress().getHostAddress() gets a NPE where it previously worked; getHostString() is the safe read; - advanced.resolve-contact-points is deprecated and inert; - fallback-to-original-contact-points defaults to true, with its cost stated; - a contact point whose hostname is unhealthy can take longer to give up on, and that cost compounds with the fallback above, since the nodes it appends are exactly the unidentified hostnames that arm the address walk; - the one-time TaggingMetricIdGenerator node-tag rename for hand-built Cloud proxy addresses; - the afterBootstrapInitialized() contract change; - two protected methods removed from internal classes that a subclass could have overridden. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MockResolverIT drives the end-to-end fix through a JVM-level InetAddress hook: a hostname that maps to one dead and one live address must still produce a working session. Its multi-address test was one change away from being vacuous. The comment claimed the dead record was tried first because of resolver insertion order, but rotate() sorts candidates by toString() and discards that order; the dead address went first only because the sort is lexicographic. The test now captures ChannelFactory at DEBUG and requires the "trying next address" event, which was proven load-bearing: moving the dead IP to one that sorts last makes it fail in 7s instead of passing in 89s. ClientRoutesIT asserts on host strings rather than resolved IPs, since a client route now stays unresolved until the connection layer expands it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ff9d8a5 to
2b64ec2
Compare
|
History regrouped, and five self-review fixes folded in. Force-pushed, so the inline threads above are now marked outdated — they all already have replies, and nothing was dropped. Why now: the branch had grown to 39 commits carrying ~1700 added lines that a later commit deleted again (churn 5949+/2143− against a net of 4246+/440−) — the withdrawn Now 9 commits, rebased onto the current
The tree is byte-identical to the reviewed head plus the five fixes below — verified by Five self-review fixes, all folded into the commit they belong to:
On the earlier suggestion to revert One consequence of the move is genuine and now documented in Verified on JDK 11: 3862 core unit tests, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
core/src/test/java/com/datastax/oss/driver/internal/core/metadata/TestNodeFactory.java:1
- Grammar in Javadoc: 'A endpoint' should be 'An endpoint'.
integration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java:1 - This test mutates the global
ChannelFactorylogger level, which can cause cross-test interference if integration tests are executed in parallel (or if another test relies on the prior level). Consider avoiding global level changes by adding a DEBUG-level appender with an appropriate filter/threshold (or a dedicated test logger name) so capture is isolated to this test instance.
| // Concatenate rather than mutate: the RUNNING-state regularQueryPlan is a built-in QueryPlan | ||
| // whose add()/addAll() throw UnsupportedOperationException (poll() is its only mutator). | ||
| // CompositeQueryPlan drains the regular plan first, then the contact-point fallback. | ||
| return new CompositeQueryPlan(regularQueryPlan, new SimpleQueryPlan(contactNodes.toArray())); |
Problem
DRIVER-201: when a contact point or a cluster node is given as a hostname that maps to multiple IPs (e.g. a DNS round-robin / dynamic-DNS entry), the driver only ever tried the first address — at initial contact, at connection time, and on control-connection reconnect. If that first IP was unreachable the driver raised
AllNodesFailedExceptioneven though the hostname also resolved to healthy IPs.This PR fixes DRIVER-201 end-to-end: every such hostname is expanded to all its addresses and each is tried in turn, for every connection the driver opens.
Design
Name resolution is a connection-layer concern.
ChannelFactory.connect()is the single place that turns "the address this node is known by" into "the addresses to actually try":EndPoint.resolve()yields one address and does no lookup — it stays safe to call from an event loop.ChannelFactoryexpands it through the bootstrap's NettyAddressResolverGroup.There is no public API change.
EndPoint.resolve()keeps its signature and is not deprecated; third-party implementations keep working unchanged. Its javadoc gains one expectation: return the address as-is rather than looking names up, since resolution now happens in the connection layer.There is, however, one behaviour change for callers of
node.getEndPoint().resolve(), documented onresolve()and in the upgrade guide: for Cloud/SNI and client-route nodes the address is now the configured hostname, unresolved, sogetAddress()returns null where it previously returned an IP. Nodes fromsystem.peersand the control node are resolved as before.getHostString()covers both.Expansion goes through Netty's resolver
Not through
InetAddress.getAllByName(). That is the resolver an unresolved address already reached when it was handed toBootstrap.connect(), so a customAddressResolverGroupinstalled viaNettyOptions.afterBootstrapInitialized()keeps applying, andBootstrap.disableResolver()is still honoured. Whether an address needs resolving at all is the resolver's decision (isSupported()/isResolved()), exactly as inBootstrap#doResolveAndConnect0— a custom resolver may report an address that already carries an IP as unresolved in order to redirect it, and it still gets that say.Consequence, unchanged from before this PR: with Netty's default resolver the lookup blocks the I/O event loop it runs on, because
DefaultNameResolvercallsInetAddress.getAllByName()inline. It is an I/O loop, never the admin loop thatconnect()is called from. Deployments that need non-blocking resolution can installDnsAddressResolverGroupand have it take effect — for the first time, for the SNI and client-route paths.One
Bootstrapand oneEventLoopare picked per logicalconnect(), and each attempt takes aclone(eventLoop)of the bootstrap. Sharing one loop between resolution and the channel keeps the group's round-robin chooser advancing exactly once per connect (taking a loop for each would park every channel on half the loops), and it means theafterBootstrapInitialized()hook runs once per logical connection rather than once per address.The candidate loop
N × connect-timeoutfor a node with N addresses. Deliberate: failing on the first unreachable IP is what this ticket is about. Real DNS entries have few records.UnsupportedProtocolVersionExceptionagainst a node whose host id is known. Every address of an identified node is that same node, so replaying the whole negotiation ladder against each remaining IP buys nothing. An unidentified endpoint — a contact point, before host ids have been read — keeps going, since one name may expand to addresses of different nodes. That preserves what collapsing a name into a singleNodewould otherwise have removed: withadvanced.resolve-contact-points = trueeach resolved address used to be its ownNode, andControlConnectionadvances its query plan on exactly this error.ChannelFactory(i.e. per session) behind a 256-entry evicting cache: the names that reach it — contact points, the SNI proxy name, client-route hostnames — are not bounded by the configuration, since client routes can hand out different hostnames on every refresh.DefaultSslEngineFactory/SniSslEngineFactorymake TLS hostname verification check the certificate against. A nameless address is worse still — reading its host name triggers a blocking reverse lookup on the event loop and validation falls back to the IP or the PTR record. So the configured name always wins, which is also what happened before multi-address support, when Netty resolved only the TCP destination and the channel kept the original endpoint. Scoped IPv6 candidates keep their zone.Pinning:
PinnableEndPointA name describes a set of addresses, but a channel is connected to exactly one.
ChannelFactorypins the endpoint to the address it used and hands that copy to the channel. This matters twice:DefaultTopologyMonitor#savePortall callresolve()on the channel's endpoint. On a pinned copy that is a field read: it neither blocks on DNS nor risks a different address than the one the channel is on.A pinned copy is otherwise indistinguishable from the original — same
equals,hashCode,asMetricPrefix()andtoString()— because nodes adopt pinned copies, andTaggingMetricIdGeneratortags node metrics with the endpoint'stoString(). The pinned address is observable only throughresolve(); which address a channel is on is in the channel's owntoString(), which Netty builds from its remote address.PinnableEndPointis internal: endpoints that do not implement it are left untouched.Where the candidate list ends
ChannelFactorywalks the candidates only while it is opening the channel. The control node's identity is read afterwards — asystem.localquery over the channel that won — so by the time that read can fail, the remaining addresses are gone.Advancing the query plan there would write off a whole hostname on the strength of one of its addresses. With a single contact point and the default
advanced.reconnect-on-init = falsethat meant initialization failed outright, and deterministically: a rebuilt session gets a freshChannelFactorywhose rotation counters start at zero, so it lands on the same address every time while a healthy one sits unused.ControlConnectiontherefore retries the same query plan entry instead, and the next attempt lands elsewhere because rotation advances once per connect. It arms only for a node with no host id whose endpoint denotes a name and whose channel reports a resolved pinned address — the same "addresses of an unidentified endpoint may belong to different nodes" reasoning as the terminal-failure rule above; an identified node stays pinned on purpose. The walk stops as soon as an address comes back round.MAX_ADDRESSES_PER_QUERY_PLAN_ENTRY(8) is only a backstop against a resolver that never repeats itself, and since it is tested before the address is recorded, one entry is attempted at most nine times.The landed address is captured as soon as the channel opens, because
resolveChannelNodeIfNeeded()overwrites the channel's endpoint with the one built from thesystem.localrow before the registration step that can still fail.Changes
Contact points stay unresolved
SessionBuilder/ContactPointsno longer resolve contact-point hostnames up front, so the query plan holds one unresolved node per contact point instead of one node per IP.advanced.resolve-contact-pointsis deprecated and has no effect. A hostname passed programmatically is expanded too, as long as theInetSocketAddressis unresolved (createUnresolved) — an already-resolved one is used as provided, which is what programmatic contact points did before this PR as well.Endpoints
DefaultEndPointreturns its address as-is, resolved or not, and implementsPinnableEndPoint.SniEndPointno longer re-resolves the proxy hostname on everyresolve()call; the connection layer expands it, so all proxy A-records are tried within one attempt and a custom Netty resolver applies. A proxy hostname is stored unresolved whichever form it arrived in, sowithCloudProxyAddress(new InetSocketAddress("proxy", 9042))— which resolves eagerly — is not frozen on one proxy IP.ClientRoutesEndPointreturns the route's hostname unresolved instead of resolving it itself, so client-route hostnames are expanded by the connection layer too.Control-connection reconnection query plan (folded from #889 review)
LoadBalancingPolicyWrapper.newControlReconnectionQueryPlan()composes the contact-point fallback asCompositeQueryPlan(regularPlan, new SimpleQueryPlan(contactNodes))instead of mutating the policy's plan: built-inQueryPlans rejectadd()/addAll(), so the previousaddAll(...)threwUnsupportedOperationExceptionon every post-init control reconnect once the fallback defaulted on. The fallback is also kept when the live-node plan is empty, even for re-resolving topology monitors, so reconnection can still recover when there is nothing else to try.NettyOptions.afterBootstrapInitializedContract documented: the driver installs its own handler afterwards, so a handler set by the hook is replaced (now warned about once), and the resolver configured there is what the driver expands names with.
OptionalLocalDcHelperRemoves the dead
checkLocalDatacenterCompatibility()check. It warned when a contact point's datacenter differed from the configured local DC, but contact-point nodes never get a datacenter assigned during refresh, so it compared againstnulland could never reflect a real mismatch — while it could fire spuriously. The separate "configured local DC matches no node" warning is retained. Unrelated to the DNS fix itself; called out because it touches aprotectedextension point.Tests
ChannelFactoryNettyResolverTest— expansion through a custom resolver,disableResolver(), an already-resolved address passed through, a resolver redirecting an already-resolved address, resolution and connect sharing one event loop, a resolver throwing synchronously.ChannelFactoryMultiAddressTest— fallback across candidates with suppressed causes, per-name/per-session/bounded rotation, hostname re-attachment (nameless, CNAME-labelled, IPv6, scoped IPv6, no-name original), and the guards that fail the connect future instead of hanging it.ChannelFactoryPinnedEndPointTest,ChannelFactoryBootstrapHookTest,ChannelFactoryProtocolNegotiationTest— pinning, the hook contract, and the terminal-vs-retryable version rejection.DefaultEndPointTest/SniEndPointTest/ClientRoutesEndPointTest— resolve/pin semantics and pin-invisible identity;DefaultNodeTest— endpoint adoption and metric-updater rebuilds;AddressUtilsTest— name vs IP literal.LoadBalancingPolicyWrapperTest— realQueryPlanstubs (the earlier mutableLinkedListstub masked the crash), plus empty-plan and re-resolving-monitor cases.ControlConnectionTest— the same-node address walk: retry on identity-read failure and on a channel closing mid-resolve, termination when an address comes back round, theMAX_ADDRESSES_PER_QUERY_PLAN_ENTRYbackstop, every failed address reported under the one node, and the two cases that must not retry (an IP literal, an already-identified node). The backstop case was checked to fail with the cap removed.MockResolverIT— end-to-end against a live cluster with a JVM-level DNS hook, including a multi-record name whose first-tried record is dead. That case capturesChannelFactoryatDEBUGand requires the "trying next address" event for the dead record, so it fails rather than passing vacuously if the candidate ordering ever stops putting the dead record first.Verified on JDK 11: full
coreunit suite (3862 tests),core+integration-testsinstall,javadoc:javadoc, a warning-free local docs build, a per-commit-Werrorcompile of all 9 commits, andMockResolverIT(3 tests) against live ScyllaDB.History
Regrouped on 2026-08-06 into 9 per-concern commits, rebased onto the current
scylla-4.xtip.The branch had grown to 39 commits carrying roughly 1700 added lines that a later commit deleted again — the withdrawn
EndPoint.resolveAll()API, a driver-owned resolver thread pool, and two test classes added and then removed. The repo rebase-merges, so all of that would have landed onscylla-4.xverbatim. Churn now equals the net diff exactly, no commit adds anything a later one deletes, and each commit compiles standalone under-Werror. The tree is unchanged by the regroup:git diffagainst the pre-regroup branch is empty.The round-by-round detail lives in the review threads below. The larger course corrections, for the record:
EndPoint.resolveAll()API and had endpoints do their own JVM DNS. Withdrawn: it bypassed a custom Netty resolver, and it put a blocking lookup behind a public method the driver calls from an event loop. Resolution moved intoChannelFactoryand the API addition was dropped, along with theresolve()deprecation and every@SuppressWarnings("deprecation")it had required.MetadataManager.getResolvedContactPoints(), its resolver executor and 3s timeout) is removed: connection-time expansion covers control-connection init and pool connections alike.On the suggestion that
fallback-to-original-contact-pointsbe reverted tofalse: its stated reason was that the flip "enables the blocking DNS fallback path by default", and that path no longer exists — the fallback appends unresolved hostnames and does no DNS at plan time. Moving resolution to the connection layer also makes the flip more necessary, not less:PinnableEndPointbinds the control node to the single address its connection reached, so it no longer re-expands, leaving the contact-point fallback as the only path back to a changed DNS record.TopologyMonitor.reresolvesNodeAddresses()documents that dependency.Deliberately not fixed here:
connect-timeouton an address it already tried. Filed as Duplicate DNS records cause repeated connection attempts to the same address #989.reference.confand the upgrade guide.DefaultNode.setEndPoint()'s clear→swap→build sequence is not atomic against concurrent metric writes: a write landing in the window goes through the cleared updater, which re-registers on demand, resurrecting one series. Strictly better than the previous ordering, which permanently deleted the new series; closing it properly means havingclearMetrics()take the ids to clear rather than recomputing them, in every metrics implementation. Documented in the code.